Skip to content

Conversation

@ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Oct 20, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Implements MSC3773

Motivation and context

Closes #6997

Thread notifications now come with sync response. This must be opted into and should also be able to fall back to previous homeserver versions that don't support the new unread thread notifications api.

Screenshots / GIFs

N/A

Tests

  • Sign into account on threads-dev.lab.element.dev
  • Get tagged in a thread using another account
  • See notification is highlighted in the timeline screen top-right icon

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 13

Checklist

@ericdecanini ericdecanini marked this pull request as ready for review October 20, 2022 23:50
@ericdecanini ericdecanini changed the base branch from feature/nfe/threads_notifications_and_receipts to develop October 20, 2022 23:50
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 2 first remarks on the form.

val threadList = it.invoke()
val isUserMentioned = threadList?.firstOrNull { threadRootEvent ->
threadRootEvent.root.threadDetails?.threadNotificationState == ThreadNotificationState.NEW_HIGHLIGHTED_MESSAGE
}?.let { true } ?: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}?.let { true } ?: false
} != null

is maybe simpler

)
)
}
val threadNotificationsSupported = session.homeServerCapabilitiesService().getHomeServerCapabilities().canUseThreadReadReceiptsAndNotifications
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping if (room == null) return would simplify the rest of the code.

# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt
@ericdecanini ericdecanini requested a review from bmarty October 24, 2022 14:32
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

10.5% 10.5% Coverage
0.0% 0.0% Duplication

@ericdecanini ericdecanini merged commit 29d3856 into develop Oct 26, 2022
@ericdecanini ericdecanini deleted the feature/eric/msc3773 branch October 26, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement MSC3773 (notifications for threads)

2 participants